Repository navigation
Relax an eager sled agent assert - #10401
Conversation
`omicron-stress` was able to trigger a sled-agent panic, and after asking for some pointers from a former very esteemed colleague, we determined that this assert was overly eager: there always was an unwrap that would enforce the same condition. Remove this assert and add a comment. Fixes oxidecomputer#10369
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
This seems reasonable to me at a glance. I wonder if we might be able to put together a test for sled-agent that reproduces the omicron-stress behavior? It would be nice to be able to have a regression test ensuring we're not just going to end up panicking later at the unwrap...
| // If `propolis_request` is Some leaving the above match group, there | ||
| // must be a Some `self.running_state`, i.e. if there is a request to | ||
| // send to the propolis process, there must be a propolis process. | ||
| // `propolis_state_put` unwraps `self.running_state` and will panic the |
There was a problem hiding this comment.
does the unwrap have a useful error message, and if not, should it be changed to an expect() with the message from the old assertion?
There was a problem hiding this comment.
yeah:
let res = self
.running_state
.as_ref()
.expect("Propolis client should be initialized before usage")
|
The |
|
Rebased, setting this as auto-merge! Sorry this was left so long |
omicron-stresswas able to trigger a sled-agent panic, and after asking for some pointers from a former very esteemed colleague, we determined that this assert was overly eager: there always was an unwrap that would enforce the same condition. Remove this assert and add a comment.Fixes #10369